Skip to content

Example of hoisting devtools dependency #1303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 19, 2021

Conversation

jonahwilliams
Copy link
Contributor

From the dependency issue post mortem, @bkonyi asked about the source of this dep. I wanted to take a look and found that it seems fairly easy to hoist into webdev. @grouma

@google-cla google-cla bot added the cla: yes label Apr 13, 2021
@@ -6,7 +6,7 @@

import 'dart:io';

import 'package:devtools_server/devtools_server.dart';
typedef DevtoolsLauncher = Future<DevTools> Function(String hostname);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this still require a dependency on DevTools given the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is just the DevTools wrapper you wrote below

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my mistake. We'll want to model this as a breaking change so we need a pubspec and changelog update. But otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thanks!

@jonahwilliams jonahwilliams requested a review from grouma April 15, 2021 17:25
@@ -15,7 +15,7 @@ dependencies:
pedantic: ^1.5.0
pub_semver: ^2.0.0
sse: ^3.8.1
web_socket_channel: ^1.0.0
web_socket_channel: ^2.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this wasn't version solving locally

@@ -1,6 +1,6 @@
name: webdev
# Every time this changes you need to run `pub run build_runner build`.
version: 2.7.2
version: 2.7.3
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a non breaking change for webdev, but I'm not sure if this is a minor or patch version bump

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor should be fine.

@jonahwilliams jonahwilliams marked this pull request as ready for review April 16, 2021 15:23
@@ -1,6 +1,6 @@
name: webdev
# Every time this changes you need to run `pub run build_runner build`.
version: 2.7.2
version: 2.7.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor should be fine.


- Support `vm_service` version `6.2.0`.
- Fix missing sdk libraries in `getObject()` calls.
- Fix incorrect `rootLib` returned by `ChromeProxyService`.
- Fix not working breakpoints in library part files.
- Fix data race in calculating locations for a module.

**Breaking changes:**
- `Dwds.start` no longer supports automatically injecting a devtools server. If `serveDevtools` is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just combine this with devtoolsLauncher? If it returns null then we know serveDevtools is false. Seems a little redundant. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jonahwilliams
Copy link
Contributor Author

oops, took out the wrong field

@jonahwilliams
Copy link
Contributor Author

Current failures might be flakes?

[e] Unhandled exception:
[e] Exception: Unable to connect to Chrome DevTools.

@grouma
Copy link
Member

grouma commented Apr 19, 2021

Current failures might be flakes?

[e] Unhandled exception:
[e] Exception: Unable to connect to Chrome DevTools.

Likely. I've found Chrome to be pretty flaky in Github Actions. Re-running to confirm.

@jonahwilliams
Copy link
Contributor Author

It passed!

@grouma grouma merged commit 699fd19 into dart-lang:master Apr 19, 2021
@jonahwilliams jonahwilliams deleted the hoist_devtools_dep branch April 19, 2021 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants